Skip to content

feat: Add implement_ops and encode pass to the toy_k2 architecture - #407

Open
PabloAndresCQ wants to merge 27 commits into
feat/toyk2_logicalsfrom
feat/toyk2_encode
Open

PabloAndresCQ wants to merge 27 commits into
feat/toyk2_logicalsfrom
feat/toyk2_encode

Conversation

@PabloAndresCQ

@PabloAndresCQ PabloAndresCQ commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #243

NOTE: There's duplication of the non-primitives dynamic ops. See the comment highlighted here: https://github.com/Quantinuum/guppyft/pull/407/changes#r4029285636. @hsemenenko has a draft PR #429 which illustrates a promising direction to avoid this.

Comment thread src/guppyft/code/toy_k2/encode.py Resolved

@hsemenenko hsemenenko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a few comments.

Comment on lines +198 to +202
@guppy
@no_type_check
def is_borrowed(self) -> bool:
"""Check if any address in the block is borrowed."""
return self.borrowed_addr[0] or self.borrowed_addr[1]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use the existing Guppy .is_borrowed method on the array?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting idea. I could also replace the borrow_addr and restore_addr by take and put.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After giving it a try, I'm not convinced this is worth doing. IMO it makes the code slightly harder to understand since you need to be aware of what put/take does on Guppy. What are there any benefits to doing this?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much butting in here but I'm not convinced by this logic - it seems like you are saying, "You'd need to be aware of what the library does, so instead I'll repeat the library here"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bigger obstacle (although perhaps only for the bigger change of put_block/take_block to use an array without Option) is that we need empty_array to initialize a STATE, from Henry's PR (but not in guppy 1.1.x)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm not sure we can use borrow-array's is_borrowed here (on a LogicalBlock) - that would mean taking the qubit out from the block, but we don't actually want to do that here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that using Guppy's is_borrowed actually doesn't fit here. What we're checking is whether a logical qubit has been borrowed from a block, but the LogicalBlock doesn't have an array of logical qubits, it has an array of physical qubits.

Comment on lines +270 to +281
# Remove the remaining addresses of this block from the stack
# NOTE: Ideally, `avail_dyn_addrs` would be a data
# structure with fast deletion (e.g. a set).
# Generally, this stack will be small, though, so it
# shouldn't be a performance bottleneck.
aux_stack: Stack[int, comptime(n_blocks)] = empty_stack()
for _ in range(len(self.avail_dyn_addrs)):
addr = self.avail_dyn_addrs.pop()
if addr[0] != blk_id:
aux_stack.push(addr)
for addr in aux_stack:
self.avail_dyn_addrs.push(addr)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead have a counter on each block to indicate how many "live" qubits there are? Once it reaches 0, all addresses are pushed back to the Stack.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current approach is for avail_dyn_addrs to be:

avail_dyn_addrs: Stack of available addresses on existing dynamic blocks.

So your suggestion "Once it reaches 0, all addresses are pushed back to the Stack." would change the semantics.
If you do what you suggest, a block that has been assigned once as dynamic is always used for dynamic qubits. The reason why avail_dyn_addrs includes only addresses on existing dynamic blocks is so that, once a dynamic block is fully freed, it may be used in "user/compiler-handled addresses mode".

Comment thread src/guppyft/code/toy_k2/encode.py Outdated

@guppy
@no_type_check
def allocate_block(self, logical_block: LogicalBlock[4] @ owned) -> int:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't allocation the block, so perhaps allocate_block_addr is more appropriate.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to use _addr because I want to reserve that to qubit addresses. I'll use allocate_block_id.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread src/guppyft/code/toy_k2/encode.py Outdated
@guppy
@no_type_check
@link_name("guppyft.toy_k2._h_dynq")
def _h_dynq(addr: tuple[int, int]) -> tuple[int, int]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If h_dynq needs an ancilla block, shouldn't we instead treat it like t-state injection where the op accepts a logical block?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things to consider:

  • Us giving inject_t the magic state as input is at the level of logical primitive. This is because we want to be able to link an opaque prep_t_state with a state factory .get_state() at implement_ops. This does not quite apply here, because h_dynq is a non-primitive, and we build it by calling _prep_zero_plus_ft, which would already call a state factory.
  • We don't need to call RUS state factories in this case, because _prep_zero_plus_ft is deterministic (note: this was not the case when you reviewed; since I used a zero state prep).

The first bullet point is yet another reason why it'd be best not to have h_dynq as a HUGR op. I'm fine with Guppy "helpers" in logical.py silently allocating qubits (we could add @expected_qubits annotations). I'm not fine with this crucial detail being hidden within an opaque op, only to have the semantics be introduced at the implement_ops level.

Comment thread src/guppyft/code/toy_k2/encode.py Resolved
Comment thread src/guppyft/code/toy_k2/encode.py Resolved
@PabloAndresCQ
PabloAndresCQ force-pushed the feat/toyk2_encode branch 14 times, most recently from 65a5283 to ce788c1 Compare October 12, 2026 13:20
@PabloAndresCQ
PabloAndresCQ marked this pull request as ready for review October 12, 2026 13:49
@PabloAndresCQ
PabloAndresCQ requested a review from a team as a code owner October 12, 2026 13:49

@hsemenenko hsemenenko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I've added some comments. Let's chat about the dynq callback op.

Comment thread src/guppyft/code/toy_k2/encode.py Outdated
# Measure the whole block because that's more efficient
m0, m1 = _measure_z_all(y_blk_id)
# But only pick the measurement outcome we care about for injection
m = m0 if addr[1] == 0 else m1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only care about m0 here as we always inject the 0th state?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Yeah, that's a bug. Either the cx_dynq in this implementation should target (y_blk_id, addr[1]) or the measurement should always use m0. The implementation in logical.py uses the former. That's an odd mistake to make when copy-pasting... I'll have a look through the _dynq implementations again, as well as logical.py to check they match.

Note: same issue in t_dynq.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed -> f8fdb6a.

I've also looked through the rest of the dynq ops and compare to the logical addressable implementations. Looks fine.

Comment thread src/guppyft/code/toy_k2/encode.py Outdated
# Measure the whole block because that's more efficient
m0, m1 = _measure_z_all(t_blk_id)
# But only pick the measurement outcome we care about for injection
m = m0 if addr[1] == 0 else m1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as s_dynq here

Comment on lines +132 to +134
for shot in res:
assert shot.keys() == {"x", "z", "s", "sdg"}
assert all(v for v in shot.values())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think checking equality with a dictionary would be better. Similarly with other tests that use all(...).

Suggested change
for shot in res:
assert shot.keys() == {"x", "z", "s", "sdg"}
assert all(v for v in shot.values())
for shot in res:
assert shot == {
"x": [1],
"z": [1],
"s": [1],
"sdg": [1],
}

@PabloAndresCQ PabloAndresCQ Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you write it as a dictionary, I realise the current all(v for v in shot.values()) is not actually testing what I intended, because Python will say that [1] is True (so would [0]) rather than tell me it's not a bool... :old-man-yells-at-python:.

I'll use dictionaries as you suggest for the sake of being explicit.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that there were some errors in the tests that were not being found due to [0] being interpreted as True. Fixes here -> cf871cf

Comment thread tests/code/toy_k2/test_toy_k2_encode.py Resolved
Comment thread tests/code/toy_k2/test_toy_k2_encode.py Resolved
Comment thread src/guppyft/code/toy_k2/encode.py Outdated

return wrapper # type: ignore[no-any-return]

lib = GuppyLibrary.from_members(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steane has added some methods to simplify this boilerplate that should be used here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool. I've updated it.

@PabloAndresCQ
PabloAndresCQ force-pushed the feat/toyk2_encode branch 3 times, most recently from 0f73856 to f8fdb6a Compare October 16, 2026 17:55
Comment thread src/guppyft/code/toy_k2/encode.py Resolved

@PabloAndresCQ PabloAndresCQ left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant changes listed below

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to _dynq ops being dropped from the logical API (see latest changes in #326), we can no longer have measure_z_dynq as a compound op of logicals (it used to compose the bindings of project_z_dynq and free_dynq. So, I'm adding it as a new extension op, since we need to compile tket2.ops.MeasureFree to it.

Comment on lines +79 to +81
@dataclass(frozen=True, kw_only=True)
class _ToyK2Options:
qed_policy: QEDPolicy

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this used EncoderParams which was removed in #465. I'm following the pattern introduced in that PR for the options of the architecture, but I have not added the annotation part of that PR.

Let me know if I should. It looked like it was not essential for ToyK2, since it's not meant to be integrated with other components.

Comment on lines +143 to +147
def prep_zero_ft_rus() -> LogicalBlock[4]:
r"""Fault-tolerant preparation of a logical :math:`|00\\rangle` state.

Unlike `prep_zero_ft`, this function will keep attempting the preparation
until it succeeds.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that the implement_ops of ToyK2 was assuming prep_zero_ft succeeded on the first attempt. My original intention was that it'd do RUS, which is why I added this primitive here (which is now used by implement_ops).

Sure, factories would be better for performance. But performance is not the goal for ToyK2, and we already have Steane for an educational example on how to implement state factories, we don't need to duplicate it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Add encode for "Toy k2" architecture

3 participants